Skip to content

Write down the conventions the codebase already follows - #62

Open
setusher wants to merge 1 commit into
masterfrom
docs/contributing-conventions
Open

Write down the conventions the codebase already follows#62
setusher wants to merge 1 commit into
masterfrom
docs/contributing-conventions

Conversation

@setusher

Copy link
Copy Markdown
Contributor

Summary

Adds CONTRIBUTING.md, recording the conventions this codebase already follows for pull request descriptions, commit messages, comments, docstrings and constants. Every rule cites the commit in omniport-backend#222 it is drawn from.

Issue

Nothing in the tree records how a change to Omniport is expected to be written. The conventions exist and are followed consistently, but only in the history, so each contributor rediscovers them from whichever files they happen to open and reviews spend their time on style rather than on substance.

The clearest statement of them is omniport-backend#222. Four of its twenty commits do nothing but bring comments and docstrings back into line, and 26d6d1a gives the reason the rest of them follow:

Cut every comment this branch adds to a single line, the reasoning behind each change belonging in its commit message rather than beside the code.

That is one rule, and most of the others are it applied somewhere. It is not written down anywhere a contributor would find it.

Steps to reproduce the bug/issue

Not reproducible at runtime, so here is the evidence in the tree instead.

  1. No conventions document exists in any Omniport repository:

    $ git ls-tree -r --name-only origin/master | grep -iE 'contributing|style|conventions'
    $ echo $?
    1

    CODE_OF_CONDUCT.md is the only governance file, and it covers behaviour rather than code.

  2. The conventions are nonetheless real and consistent. Every subject line in #222 is imperative, sentence case, no prefix, no ticket number:

    Move password recovery constants to a constants module
    Let Django own the security response headers
    Keep serving password recovery over GET while it is deprecated
    
  3. And they are enforced by hand, after the fact, which is the cost of not writing them down. #222 contains four separate cleanup commits doing it: 79f4a68, 2776810, 26d6d1a, ff40f42.

Steps done to fix it and test added for the same

CONTRIBUTING.md covers, in this order: the pull request description, commits, comments, docstrings, constants, code and prose.

The rules are taken from the history rather than invented, and each cites its commit so a reader who disagrees can go and read the change:

Rule From Evidence
One line per comment, reasoning in the commit message 26d6d1a a three-line comment cut to one
Why, never what; no capitals for emphasis 26d6d1a # ALWAYS return identical response - CRITICAL ... became # The same response either way, so that accounts cannot be enumerated
Match what the neighbours do ff40f42 a correct, useful comment deleted because the list carries none
No CWE listings or mitigation bullets in a docstring 79f4a68 a six-line SECURITY FIXES block cut to two lines
A docstring must not restate a value that lives in code 79f4a68 the rate limit figures dropped, the constants module being where they cannot go stale
Prose commit bodies, no bullet lists dcc00d0 quoted in full as the model
Constants grouped in a constants module 5830ac1 the last constants living inside a middleware module moved out

The pull request headings are the five .github/pull_request_template.md already uses, written out with what each section is for, plus the rule that a test has to run in CI and be observed failing before it is believed.

No test is added, and that is a deliberate choice worth stating. Almost nothing here is mechanically checkable: whether a comment explains the why rather than the what, or whether a commit body names the mechanism, is a judgement. Adding a linter that checks the two rules which are mechanical, the em dash and the subject line shape, would give the impression the rest are enforced too. The document says so itself, and prefers a mechanical check wherever the property allows one.

What was done instead is check the document against its own rules, which found two failures and fixed both:

em dashes                     1 -> 0
prose lines with >1 sentence  27 -> 0

A style guide that breaks its own rules gets ignored.

Criteria for issue to be resolved

  • CONTRIBUTING.md is at the repository root, where GitHub links it from the pull request and issue forms
  • Every commit hash it cites resolves in omniport-backend: dcc00d0, 4164fc0, ff40f42, 26d6d1a, 79f4a68, 5830ac1
  • The five headings match .github/pull_request_template.md exactly
  • The document contains no em dash, no agent or tool attribution, and no pull request or issue number outside a "do not write this" example
  • Every prose line carries one sentence
  • Code fences are balanced and the before/after pairs match the real diffs in #222
  • A reviewer can point at a rule and, from the hash beside it, read the change that established it

Scope

This lands in omniport-docker alone, as the repository every contributor already has checked out, rather than being copied into all ninety-odd repositories where it would drift. If it should also sit in omniport-backend, where #222 actually lives, that is a one-file follow-up.

The Co-Authored-By trailer that every commit in #222 carries is not written down as a convention here, deliberately. It is house practice by the evidence, but attribution is a policy question rather than a style one, and this document should not be the place it gets settled.

Nothing recorded how a change to Omniport is expected to be written, so
every contributor rediscovered it from whichever files they happened to
read, and reviews spent their time on style rather than on substance.

omniport-backend#222 is the clearest statement of the conventions in the
history. Four of its twenty commits do nothing but bring comments and
docstrings back into line, and the reason it gives is the rule the rest
of them follow: the reasoning behind a change belongs in its commit
message rather than beside the code. Every rule here is that applied
somewhere, and each one cites the commit it comes from, so a reader who
disagrees can go and read the change rather than argue with a document.

The pull request headings are the five already used by
.github/pull_request_template.md, written out with what each is for.

CONTRIBUTING.md rather than a wiki page, because GitHub links it from
the pull request and issue forms of the repository it sits in, and
because a convention that is not in the tree goes stale without anyone
noticing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant